-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor RustTarget
#2993
Refactor RustTarget
#2993
Conversation
aa7ba97
to
43d5d32
Compare
43d5d32
to
9117083
Compare
9117083
to
5f6c6cd
Compare
Thanks for this, Christian! Just to confirm: for projects that need to support |
Yes all the previous valid inputs are still valid, e.g. |
Sounds great, thanks for confirming! The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would've been nice to send the removal of deprecated targets separately, but looks good! Some minor bits only.
This is done because bindgen is producing bogus code where a single struct has both `packed` and `align` attributes.
5f7c9d5
to
2d68245
Compare
Yes, they'd have to add extra |
Each `bindgen` release may upgrade the list of Rust targets. For instance, currently, in their master branch [1], the latest ones are: Nightly => { vectorcall_abi: #124485, ptr_metadata: #81513, layout_for_ptr: #69835, }, Stable_1_77(77) => { offset_of: #106655 }, Stable_1_73(73) => { thiscall_abi: #42202 }, Stable_1_71(71) => { c_unwind_abi: #106075 }, Stable_1_68(68) => { abi_efiapi: #105795 }, By default, the highest stable release in their list is used, and users are expected to set one if they need to support older Rust versions (e.g. see [2]). Thus, over time, new Rust features are used by default, and at some point, it is likely that `bindgen` will emit Rust code that requires a Rust version higher than our minimum (or perhaps enabling an unstable feature). Currently, there is no problem because the maximum they have, as seen above, is Rust 1.77.0, and our current minimum is Rust 1.78.0. Therefore, set a Rust target explicitly now to prevent going forward in time too much and thus getting potential build failures at some point. Since we also support a minimum `bindgen` version, and since `bindgen` does not support passing unknown Rust target versions, we need to use the list of our minimum `bindgen` version, rather than the latest. So, since `bindgen` 0.65.1 had this list [3], we need to use Rust 1.68.0: /// Rust stable 1.64 /// * `core_ffi_c` ([Tracking issue](rust-lang/rust#94501)) => Stable_1_64 => 1.64; /// Rust stable 1.68 /// * `abi_efiapi` calling convention ([Tracking issue](rust-lang/rust#65815)) => Stable_1_68 => 1.68; /// Nightly rust /// * `thiscall` calling convention ([Tracking issue](rust-lang/rust#42202)) /// * `vectorcall` calling convention (no tracking issue) /// * `c_unwind` calling convention ([Tracking issue](rust-lang/rust#74990)) => Nightly => nightly; ... /// Latest stable release of Rust pub const LATEST_STABLE_RUST: RustTarget = RustTarget::Stable_1_68; Thus add the `--rust-target 1.68` parameter. Add a comment as well explaining this. An alternative would be to use the currently running (i.e. actual) `rustc` and `bindgen` versions to pick a "better" Rust target version. However, that would introduce more moving parts depending on the user setup and is also more complex to implement. Starting with `bindgen` 0.71.0 [4], we will be able to set any future Rust version instead, i.e. we will be able to set here our minimum supported Rust version. Christian implemented it [5] after seeing this patch. Thanks! Cc: [email protected] # 6.12.y only (since older LTSs only support a single `bindgen` version) Cc: Christian Poveda <[email protected]> Cc: Emilio Cobos Álvarez <[email protected]> Link: https://github.com/rust-lang/rust-bindgen/blob/21c60f473f4e824d4aa9b2b508056320d474b110/bindgen/features.rs#L97-L105 [1] Link: rust-lang/rust-bindgen#2960 [2] Link: https://github.com/rust-lang/rust-bindgen/blob/7d243056d335fdc4537f7bca73c06d01aae24ddc/bindgen/features.rs#L131-L150 [3] Link: https://github.com/rust-lang/rust-bindgen/blob/main/CHANGELOG.md#0710-2024-12-06 [4] Link: rust-lang/rust-bindgen#2993 [5] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>
Each `bindgen` release may upgrade the list of Rust targets. For instance, currently, in their master branch [1], the latest ones are: Nightly => { vectorcall_abi: #124485, ptr_metadata: #81513, layout_for_ptr: #69835, }, Stable_1_77(77) => { offset_of: #106655 }, Stable_1_73(73) => { thiscall_abi: #42202 }, Stable_1_71(71) => { c_unwind_abi: #106075 }, Stable_1_68(68) => { abi_efiapi: #105795 }, By default, the highest stable release in their list is used, and users are expected to set one if they need to support older Rust versions (e.g. see [2]). Thus, over time, new Rust features are used by default, and at some point, it is likely that `bindgen` will emit Rust code that requires a Rust version higher than our minimum (or perhaps enabling an unstable feature). Currently, there is no problem because the maximum they have, as seen above, is Rust 1.77.0, and our current minimum is Rust 1.78.0. Therefore, set a Rust target explicitly now to prevent going forward in time too much and thus getting potential build failures at some point. Since we also support a minimum `bindgen` version, and since `bindgen` does not support passing unknown Rust target versions, we need to use the list of our minimum `bindgen` version, rather than the latest. So, since `bindgen` 0.65.1 had this list [3], we need to use Rust 1.68.0: /// Rust stable 1.64 /// * `core_ffi_c` ([Tracking issue](rust-lang/rust#94501)) => Stable_1_64 => 1.64; /// Rust stable 1.68 /// * `abi_efiapi` calling convention ([Tracking issue](rust-lang/rust#65815)) => Stable_1_68 => 1.68; /// Nightly rust /// * `thiscall` calling convention ([Tracking issue](rust-lang/rust#42202)) /// * `vectorcall` calling convention (no tracking issue) /// * `c_unwind` calling convention ([Tracking issue](rust-lang/rust#74990)) => Nightly => nightly; ... /// Latest stable release of Rust pub const LATEST_STABLE_RUST: RustTarget = RustTarget::Stable_1_68; Thus add the `--rust-target 1.68` parameter. Add a comment as well explaining this. An alternative would be to use the currently running (i.e. actual) `rustc` and `bindgen` versions to pick a "better" Rust target version. However, that would introduce more moving parts depending on the user setup and is also more complex to implement. Starting with `bindgen` 0.71.0 [4], we will be able to set any future Rust version instead, i.e. we will be able to set here our minimum supported Rust version. Christian implemented it [5] after seeing this patch. Thanks! Cc: Christian Poveda <[email protected]> Cc: Emilio Cobos Álvarez <[email protected]> Cc: [email protected] # needed for 6.12.y; unneeded for 6.6.y; do not apply to 6.1.y Fixes: c844fa6 ("rust: start supporting several `bindgen` versions") Link: https://github.com/rust-lang/rust-bindgen/blob/21c60f473f4e824d4aa9b2b508056320d474b110/bindgen/features.rs#L97-L105 [1] Link: rust-lang/rust-bindgen#2960 [2] Link: https://github.com/rust-lang/rust-bindgen/blob/7d243056d335fdc4537f7bca73c06d01aae24ddc/bindgen/features.rs#L131-L150 [3] Link: https://github.com/rust-lang/rust-bindgen/blob/main/CHANGELOG.md#0710-2024-12-06 [4] Link: rust-lang/rust-bindgen#2993 [5] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>
This PR refactors the
RustTarget
type to be more flexible.The motivation behind this change comes from the current definition of
RustTarget
as an enum. Which forces users to pick a specific version of Rust that might not match the msrv of their projects exactly.As an example, let's say that a project using bindgen has an msrv of
1.41.1
and they decide to use the--rust-target
flag to guarantee that bindgen won't produce code that breaks their msrv.Given that bindgen doesn't have a
RustTarget
variant that specifically matches1.41.1
, they have to use the closestRustTarget
that is still compatible. In this case that would be1.40
. If they decide to change their msrv in the future, they have to repeat this search process with their new msrv.With the changes done in this PR, it would be possible to pass any* Rust version to
--rust-target
and bindgen will automatically enable the Rust features that are compatible with that version. In our example, that means that it would be possible to use--rust-version=1.41.1
explicitly, or even better, read the value fromrust-version
so they don't have to manually change it in the future.The specific changes done in this PR are:
Replace the
RustTarget
enum for an opaque struct that can represent any* Rust version.Update the
FromStr
implementation forRustTarget
so it can parse any* Rust version.Introduce several deprecated constants under
RustTarget
so people can still use those as if they were still using the variants of the removed enum type.Introduce the builders
RustTarget::nightly
andRustTarget::stable
.Remove all the deprecated
RustTarget
s.Update the expectation tests to accommodate the removal of the deprecated targets.
(*): Unless it's not supported by bindgen anymore. To the date, anything earlier than 1.33 (inclusive)
cc @ojeda